-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Refactor File.Content to use discriminated union with MIME types #3108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Refactor File.Content to use discriminated union with MIME types #3108
Conversation
@kmk142789 please stop reviewing prs, the review doesnt do anything since you arent a maintainer and it can lead to confusion with those trying to contribute |
@kcrommett ill peep this today thanks! |
packages/opencode/src/file/index.ts
Outdated
const mimeTypes: Record<string, string> = { | ||
".jpg": "image/jpeg", | ||
".jpeg": "image/jpeg", | ||
".png": "image/png", | ||
".gif": "image/gif", | ||
".bmp": "image/bmp", | ||
".webp": "image/webp", | ||
".ico": "image/x-icon", | ||
".svg": "image/svg+xml", | ||
".zip": "application/zip", | ||
".tar": "application/x-tar", | ||
".gz": "application/gzip", | ||
".pdf": "application/pdf", | ||
".wasm": "application/wasm", | ||
".exe": "application/x-msdownload", | ||
".dll": "application/x-msdownload", | ||
".so": "application/x-sharedlib", | ||
} | ||
return mimeTypes[ext] || "application/octet-stream" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the bun api return the mime type automatically?
const content = Buffer.from(buffer).toString("base64") | ||
return { type: "binary", content, mimeType: getMimeType(full) } | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For images would it be best to base64 encode them here? Should we return raw bytes?
Idk maybe we should take some inspiration from S3 on this one...
Whats the most common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sticking with base64. We’re shipping file content inside a JSON payload so it needs to be text-safe, and base64 is the de facto approach (AWS S3’s JSON APIs do the same). Returning raw bytes would force a binary response instead of the existing schema, so we’d lose compatibility with the current endpoint. With the new encoding: "base64" flag, clients can reliably decode via Buffer.from(content, "base64") or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet ill test your latest changes and then this should be good to go.
Summary
File.Content
to a discriminated union for type-safe text vs binaryhandling
cs
Changes
File.Content
with a discriminated union keyed bytype
:text
:{ type: "text", content, diff?, patch? }
binary
:{ type: "binary", content /* base64 */, mimeType }
getMimeType(path)
with common extension map andapplication/octet-strea m
fallbackisBinaryFile(path, file)
using extension heuristics plus a null-byte scan of the first up to 512 bytes
read(path)
:binary
with base64 content andmimeType
for detected binariestext
content; when tracked in git, computediff
and fullpatch
(tries unstaged, then staged)
FileContent
as a discriminated unionBenefits
ver and SDK
mimeType
metadata enables correct client-side rendering and processing
text
variant; binary is base64 with metadatacontent.type
discriminator